Refactor GraphCache to support multiple types for the same topic. #70

Yadunund · 2023-11-24T09:17:19Z

This PR

Updates the GraphCache implementation to support multiple types over the same topic name.
Previously we were not keeping track of multiple pub/subs in the same node that publish over the same topic and let alone with different topic types. This PR addresses both these problems as well.
It also reduces code duplication when adding/removing pub/subs by relying on lambdas that are reused for both pub/subs.
Adds data structures and methods (see liveliness_utils.hpp) to manage token generation and conversion to C++ types. No longer have any magic string literal comparisons or custom key generation for unordered_maps.

Signed-off-by: Yadunund <[email protected]>

gavanderhoorn · 2023-11-24T09:27:59Z

This PR updates the GraphCache implementation to support multiple types over the same topic name.

I'm just watching this repository, so likely out-of-the-loop a bit, but what would be a use-case for this? Wouldn't that break the "strong typing" of ROS topics?

clalancette · 2023-11-25T14:46:53Z

I'm just watching this repository, so likely out-of-the-loop a bit, but what would be a use-case for this? Wouldn't that break the "strong typing" of ROS topics?

This is a supported use-case today:

Terminal 1:

$ ros2 topic pub -r 1 /foo std_msgs/msg/String '{data: hello}'

Terminal 2:

$ ros2 topic pub -r 1 /foo std_msgs/msg/Int32 '{data: 3}'

Terminal 3:

$ ros2 topic list -t
/foo [std_msgs/msg/Int32, std_msgs/msg/String]
/parameter_events [rcl_interfaces/msg/ParameterEvent]
/rosout [rcl_interfaces/msg/Log]

Is it a good idea? No. But since the aim is to keep most of the semantics the same with rmw_zenoh_cpp, we should support it.

gavanderhoorn · 2023-11-27T08:41:39Z

This is a supported use-case today

yes, I know ..

I still can't really come up with a good use-case for this though.

Is it a good idea? No.

Indeed.

Signed-off-by: Yadunund <[email protected]>

Yadunund · 2023-11-27T15:15:56Z

I'm just watching this repository, so likely out-of-the-loop a bit, but what would be a use-case for this? Wouldn't that break the "strong typing" of ROS topics?

This is a supported use-case today:

Terminal 1:
$ ros2 topic pub -r 1 /foo std_msgs/msg/String '{data: hello}'
Terminal 2:
$ ros2 topic pub -r 1 /foo std_msgs/msg/Int32 '{data: 3}'
Terminal 3:
$ ros2 topic list -t
/foo [std_msgs/msg/Int32, std_msgs/msg/String]
/parameter_events [rcl_interfaces/msg/ParameterEvent]
/rosout [rcl_interfaces/msg/Log]
Is it a good idea? No. But since the aim is to keep most of the semantics the same with rmw_zenoh_cpp, we should support it.

@clalancette this should work with PR now. Could you give it a spin?
Edit: ros2 topic list -v should also be supported now

Signed-off-by: Yadunund <[email protected]>

Signed-off-by: Chris Lalancette <[email protected]>

Also fix a couple of bugs where we were mixing up subscriptions and publications, leading to incorrect counts. Signed-off-by: Chris Lalancette <[email protected]>

Because it returned a reference, it is difficult to tell the lifetime of it. It would also be hard to add in locking in the future. Since we are reaching through it in most cases, just return some data directly. Signed-off-by: Chris Lalancette <[email protected]>

Signed-off-by: Chris Lalancette <[email protected]>

clalancette

Overall, this is looking pretty good.

Inline, I've left one major piece of feedback about APIs.

Besides that, I'm noticing segmentation faults and errors when I shut nodes down with this. I suspect that we are trying to process some liveliness tokens while we are shutting down the context, but I'm not sure. I think we should probably address that before merging this in.

clalancette · 2023-11-27T18:08:29Z

rmw_zenoh_cpp/src/detail/liveliness_utils.cpp

+const std::optional<TopicInfo> & Entity::topic_info() const
+{
+  return this->topic_info_;
+}


I'm not a huge fan of APIs like this, that return a reference to something inside of the class.

There are two (related) reasons:

It becomes difficult to tell what the lifetime of this returned value is. If the caller took a reference, but then this class gets destroyed, it is now holding onto a dangling reference.

If we want to add in locking around topic_info_, for instance, it becomes tricky; we have to expose the locking mechanism outside of the class to all callers, and then all callers have to remember to actually lock it.

You can see that I actually changed node_info above to three separate APIs that just return the needed data without references. That is much safer, in my opinion, and I think we should do a similar thing for topic_info(), type(), and keyexpr() in this PR.

Thanks for the feedback! I've pushed 4488b97. I hope it's fine to let topic_info()still return an std::optional<TopicInfo>. Else if we split into 3 APIs, we will need to check all 3 params are set each time we want to parse necessary data for pub/subs. To prevent large copies of strings each time, I've updated the implementation to make one copy of topic_info() and access the required fields where ever possible.

Besides that, I'm noticing segmentation faults and errors when I shut nodes down with this. I suspect that we are trying to process some liveliness tokens while we are shutting down the context, but I'm not sure. I think we should probably address that before merging this in.

Do you have any commands to reproduce this behavior? I wasn't able to do so. But I did encounter this problem early on when I had the ros2 daemon running from an older commit of this branch. After ros2 daemon stop and rebuilding I never saw that again.

Signed-off-by: Yadunund <[email protected]>

francocipollone

Great stuff! 🚀

I left some minor comments

I tested locally running demo nodes:

(docker) franco@:~/ros_dev_ws$ ros2 topic list -v
Published topics:
 * /chatter [std_msgs/msg/String] 1 publisher
 * /parameter_events [rcl_interfaces/msg/ParameterEvent] 3 publishers
 * /rosout [rcl_interfaces/msg/Log] 3 publishers

Subscribed topics:
 * /chatter [std_msgs/msg/String] 1 subscriber
 * /parameter_events [rcl_interfaces/msg/ParameterEvent] 2 subscribers

I haven't experienced any error when shutting down the nodes

francocipollone · 2023-11-29T18:59:57Z

rmw_zenoh_cpp/src/detail/graph_cache.cpp

+  std::optional<liveliness::Entity> valid_entity = liveliness::Entity::make(keyexpr);
+  if (!valid_entity.has_value()) {
+    // Error message has already been logged.
+    return;
+  }


Regarding error handling.

When the entity can't be created, the error msg is set and then it is returned. However, when the method (parse_put) is being called there is no way to know it failed from the caller's perspective.

It can be argued how to deal with errors with discovery and whether that should throw and exception or continue the process if the data transport is still working. Maybe something we can iterate on as things get clearer?

Opened a ticket to work on in the next iteration #72

francocipollone · 2023-11-29T19:10:37Z

rmw_zenoh_cpp/src/detail/liveliness_utils.cpp

+        "rmw_zenoh_cpp",
+        "Received liveliness token for non-node entity without required parameters.");
+      return std::nullopt;
+    }


nit: Just for consistency, should we check here if we have strictly more than 8 parts? We shouldn't reach that part though.

I'd be in favor of not having any magic numbers like 8 anywhere in the code. Maybe we can update the Entity API to return the number of parts for a given EntityType.

Ticketed to work on in the next iteration #71

francocipollone · 2023-11-29T19:22:31Z

rmw_zenoh_cpp/src/detail/graph_cache.cpp

+  if (ns_it == graph_.end()) {
+    NodeMap node_map = {
+      {entity.node_name(), make_graph_node(entity)}};
+    graph_.insert(std::make_pair(entity.node_namespace(), std::move(node_map)));


nit: using emplace instead of insert might be useful to not rely on the compiler for optimization in the construction/copy of the object

Emplace creates the object in-place instead of moving it with insert if possible.

https://en.cppreference.com/w/cpp/container/unordered_map/emplace

Done 357f658

rmw_zenoh_cpp/src/detail/graph_cache.cpp

francocipollone · 2023-11-29T19:46:02Z

rmw_zenoh_cpp/src/detail/graph_cache.cpp

+        auto topic_data_ptr = std::make_shared<TopicData>(
+          topic_info,
+          TopicStats{pub_count, sub_count}
+        );
+        graph_topics_[topic_info.name_] = GraphNode::TopicDataMap{
+          {topic_info.type_, topic_data_ptr}
+        };


Isn't it the same as topic_data_map?

Suggested change

auto topic_data_ptr = std::make_shared<TopicData>(

topic_info,

TopicStats{pub_count, sub_count}

);

graph_topics_[topic_info.name_] = GraphNode::TopicDataMap{

{topic_info.type_, topic_data_ptr}

};

graph_topics_[topic_info.name_] = topic_data_map;

Done 357f658

francocipollone · 2023-11-29T19:53:59Z

rmw_zenoh_cpp/src/detail/graph_cache.hpp

+  NamespaceMap graph_ = {};
+
+  // Optimize topic lookups across the graph.
+  GraphNode::TopicMap graph_topics_ = {};


Just for documentation's sake. Without this optimized collection for topic lookups, does the time for the queries(for topics) go up considerably? Just wondering why solving the query on demand isn't a chance. Taking into account that keeping the double source of truth might be error-prone.

With the cache, lookups to check if a certain topic exists in the graph have O(1) complexity on average. Else if we have to iterate through graph_cache_, it will be O(n*n'*n'') where we have to iterate through n namepaces, n' nodes within each namespace and potentially n'' topics per node.

Signed-off-by: Yadunund <[email protected]>

Yadunund · 2023-12-02T00:31:32Z

Thanks for the review @clalancette and @francocipollone! I've addressed the blocking feedback and opened tickets for the others. Will merge this in now to continue impl of introspection methods.

* use std::find_if to find matching pubs Signed-off-by: Yadunund <[email protected]> * Refactor to support multiple types Signed-off-by: Yadunund <[email protected]> * Refactor liveliness token generation Signed-off-by: Yadunund <[email protected]> * Map topic types to TopicData Signed-off-by: Yadunund <[email protected]> * Store map of types in graph_topics_ Signed-off-by: Yadunund <[email protected]> * fix bug in deleting and counting topics Signed-off-by: Yadunund <[email protected]> * Cleanup liveliness_utils Signed-off-by: Yadunund <[email protected]> * Style cleanups. Signed-off-by: Chris Lalancette <[email protected]> * Cleanup some of the code in the liveliness. Also fix a couple of bugs where we were mixing up subscriptions and publications, leading to incorrect counts. Signed-off-by: Chris Lalancette <[email protected]> * Remove Entity::node_info. Because it returned a reference, it is difficult to tell the lifetime of it. It would also be hard to add in locking in the future. Since we are reaching through it in most cases, just return some data directly. Signed-off-by: Chris Lalancette <[email protected]> * Remove some uses of auto. Signed-off-by: Chris Lalancette <[email protected]> * Return copies from Entity Signed-off-by: Yadunund <[email protected]> * Address feedback Signed-off-by: Yadunund <[email protected]> --------- Signed-off-by: Yadunund <[email protected]> Signed-off-by: Yadunund <[email protected]> Signed-off-by: Chris Lalancette <[email protected]> Co-authored-by: Chris Lalancette <[email protected]>

Yadunund added 2 commits November 23, 2023 14:01

use std::find_if to find matching pubs

adc0a2f

Signed-off-by: Yadunund <[email protected]>

Refactor to support multiple types

d99e776

Signed-off-by: Yadunund <[email protected]>

Yadunund requested review from clalancette and francocipollone November 24, 2023 09:17

Yadunund added 3 commits November 27, 2023 16:17

Refactor liveliness token generation

208f3ef

Signed-off-by: Yadunund <[email protected]>

Map topic types to TopicData

7268001

Signed-off-by: Yadunund <[email protected]>

Store map of types in graph_topics_

64a023d

Signed-off-by: Yadunund <[email protected]>

Yadunund and others added 5 commits November 27, 2023 23:47

fix bug in deleting and counting topics

c4fa36c

Signed-off-by: Yadunund <[email protected]>

Cleanup liveliness_utils

6bc2f39

Signed-off-by: Yadunund <[email protected]>

Style cleanups.

7f6fa1f

Signed-off-by: Chris Lalancette <[email protected]>

Cleanup some of the code in the liveliness.

7c79ff1

Also fix a couple of bugs where we were mixing up subscriptions and publications, leading to incorrect counts. Signed-off-by: Chris Lalancette <[email protected]>

clalancette force-pushed the yadu/support_multiple_topic_types branch from ae0b4d9 to 178cc38 Compare November 27, 2023 18:10

Remove some uses of auto.

b26a268

Signed-off-by: Chris Lalancette <[email protected]>

clalancette requested changes Nov 27, 2023

View reviewed changes

Return copies from Entity

4488b97

Signed-off-by: Yadunund <[email protected]>

francocipollone reviewed Nov 29, 2023

View reviewed changes

Address feedback

357f658

Signed-off-by: Yadunund <[email protected]>

This was referenced Dec 2, 2023

Update Entity API to return number of parts in keyexpression #71

Closed

Handle errors in parsing liveliness tokens #72

Closed

Investigate potential segfault when node is shutdown #73

Closed

Yadunund merged commit 1bfc2c1 into rolling Dec 2, 2023
5 checks passed

delete-merged-branch bot deleted the yadu/support_multiple_topic_types branch December 2, 2023 00:31

Yadunund mentioned this pull request Dec 2, 2023

[Fix PR #70] Instantiate new topic data for cache #75

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor GraphCache to support multiple types for the same topic. #70

Refactor GraphCache to support multiple types for the same topic. #70

Yadunund commented Nov 24, 2023 •

edited

Loading

gavanderhoorn commented Nov 24, 2023 •

edited

Loading

clalancette commented Nov 25, 2023

gavanderhoorn commented Nov 27, 2023 •

edited

Loading

Yadunund commented Nov 27, 2023 •

edited

Loading

clalancette left a comment

clalancette Nov 27, 2023

Yadunund Nov 28, 2023 •

edited

Loading

francocipollone left a comment

francocipollone Nov 29, 2023

Yadunund Dec 1, 2023

Yadunund Dec 2, 2023

francocipollone Nov 29, 2023

Yadunund Dec 1, 2023

Yadunund Dec 2, 2023

francocipollone Nov 29, 2023

Yadunund Dec 2, 2023

francocipollone Nov 29, 2023

Yadunund Dec 2, 2023

francocipollone Nov 29, 2023

Yadunund Dec 1, 2023

Yadunund commented Dec 2, 2023

Refactor GraphCache to support multiple types for the same topic. #70

Refactor GraphCache to support multiple types for the same topic. #70

Conversation

Yadunund commented Nov 24, 2023 • edited Loading

gavanderhoorn commented Nov 24, 2023 • edited Loading

clalancette commented Nov 25, 2023

gavanderhoorn commented Nov 27, 2023 • edited Loading

Yadunund commented Nov 27, 2023 • edited Loading

clalancette left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yadunund Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

francocipollone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yadunund commented Dec 2, 2023

Yadunund commented Nov 24, 2023 •

edited

Loading

gavanderhoorn commented Nov 24, 2023 •

edited

Loading

gavanderhoorn commented Nov 27, 2023 •

edited

Loading

Yadunund commented Nov 27, 2023 •

edited

Loading

Yadunund Nov 28, 2023 •

edited

Loading